Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ADP-478: Garbage collect delisted stake pools from SMASH #2249

Merged
merged 29 commits into from
Nov 10, 2020

Conversation

hasufell
Copy link
Contributor

@hasufell hasufell commented Oct 15, 2020

This is the first step for garbage collecting
stake pools based on SMASH delisting.

X-JIRA-Ticket: ADP-478


Questions / Considerations

  1. After looking at the logic, I believe we don't have to adjust any other functions of the DBLayer such as listRegisteredPools or readPoolProduction. These can still consider all pools. Only the API layer will consider delisted pools and adjust ApiStakePool accordingly.
  2. There's still the open question whether we want to really delete GCed pools via removePools at some point.

TODO

Feature

  • Add delisted column to metadata table and populate Pool DBLayer with functions
  • Create new internal_state table and have GC thread store last sync time
  • Add garbage collection thread querying the SMASH server for delisted pools and calling delistPools from DBLayer
  • have GC thread store last sync time
  • return last GC sync time
  • Add POST '{ "maintenance_action": "gc_stake_pools" }' /stake-pools endpoint

QA

  • database tests
  • check json roundtripping works
  • Add integration tests? We don't have SMASH server integration tests yet.

@hasufell hasufell requested a review from KtorZ October 15, 2020 15:52
@hasufell hasufell force-pushed the jospald/ADP-478/gc-delisted-stake-pools branch from 865c6eb to 0ed2886 Compare October 15, 2020 16:12
@hasufell hasufell changed the title Add delistPools to Pool DBLayer [WIP] Add delistPools to Pool DBLayer Oct 15, 2020
@hasufell hasufell marked this pull request as draft October 15, 2020 16:22
@hasufell hasufell force-pushed the jospald/ADP-478/gc-delisted-stake-pools branch 3 times, most recently from b298bb2 to c0dc034 Compare October 16, 2020 15:34
@hasufell hasufell marked this pull request as ready for review October 17, 2020 14:32
@hasufell hasufell force-pushed the jospald/ADP-478/gc-delisted-stake-pools branch from b42eb40 to 9147481 Compare October 17, 2020 14:43
@hasufell hasufell changed the title [WIP] Add delistPools to Pool DBLayer Add delistPools to Pool DBLayer Oct 17, 2020
iohk-bors bot added a commit that referenced this pull request Oct 17, 2020
@hasufell hasufell force-pushed the jospald/ADP-478/gc-delisted-stake-pools branch 2 times, most recently from a03670c to 6327f88 Compare October 17, 2020 16:43
@cardano-foundation cardano-foundation deleted a comment from iohk-bors bot Oct 17, 2020
iohk-bors bot added a commit that referenced this pull request Oct 17, 2020
@hasufell hasufell force-pushed the jospald/ADP-478/gc-delisted-stake-pools branch from 6327f88 to 59a75fe Compare October 17, 2020 21:07
@cardano-foundation cardano-foundation deleted a comment from iohk-bors bot Oct 17, 2020
iohk-bors bot added a commit that referenced this pull request Oct 17, 2020
iohk-bors bot added a commit that referenced this pull request Oct 19, 2020
@hasufell hasufell force-pushed the jospald/ADP-478/gc-delisted-stake-pools branch from 59a75fe to 5d0c0a6 Compare October 19, 2020 13:28
@hasufell
Copy link
Contributor Author

bors try

iohk-bors bot added a commit that referenced this pull request Oct 19, 2020
@hasufell hasufell changed the title Add delistPools to Pool DBLayer ADP-478: Garbage collect delisted stake pools from SMASH Oct 19, 2020
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 19, 2020

try

Build failed:

@piotr-iohk
Copy link
Contributor

Add integration tests? We don't have SMASH server integration tests yet.

Perhaps we could have a mock SMASH server in the integration tests? Maybe it would be enough as it just mock the situation that there is one stake pool blacklisted. This way we could make a system level test.

Currently in our integration suite there are 4 pools being registered. One of them is retired at the very beginning (before the tests start). Two out of remaining three are a scheduled to be retired in the far future (and they are like that for the span of the suite). We could extend this set up to have yet another pool, but this one would served as "blacklisted" in the mock SMASH server and we would just make sure that this pool is not visible when we have metadata fetching strategy pointing to a mock SMASH server.

We used to have a mock server for jormungandr metadata registry, that was serving a zip file with metadata https://github.com/input-output-hk/cardano-wallet/blob/master/lib/jormungandr/test/integration/Main.hs#L230-L238... Maybe similar concept could be used.

@hasufell hasufell self-assigned this Oct 20, 2020
@cardano-foundation cardano-foundation deleted a comment from iohk-bors bot Oct 20, 2020
@cardano-foundation cardano-foundation deleted a comment from iohk-bors bot Oct 20, 2020
jonathanknowles and others added 10 commits November 10, 2020 09:56
Record delisted pools in a dedicated table instead of using a field in
the `pool_registrations` table.

In the updated schema, a pool is delisted if (and only if) there is a
single row containing that pool's id in the `delisted_pools` table.

This solution has several advantages:

  1.  We only need a single database row to record that a pool is delisted.

  2.  We no longer need to carefully to ensure that all registration records
      for a particular pool have the same delisted status. A pool is either
      delisted or not delisted: the schema rules out all intermediate states.

  3.  Pools automatically remain delisted when rollbacks occur or when new
      certificates are published, with no extra effort.

  4.  The `putPoolRegistration` function no longer needs to read the
      most-recently-written registration entry before adding a new entry.

  5.  Each row in the `pool_registrations` table is now just an immutable
      record of a registration certificate.

  6.  The `PoolFlag` type is no longer necessary.
We must replace the set of delisted pools, rather than augmenting it.

In response to review feedback:

#2277 (comment)
This operation completely replaces the set of delisted pools, rather
than augmenting it. Therefore, the name `delistPools` is slightly
misleading, as it gives the impression that the existing set will be
augmented, which is no longer true.

In response to review feedback:

#2277 (comment)
In particular, we check that 'putDelistedPools' completely overwrites
the existing set every time.

In response to review feedback:

#2277 (comment)
This test no longer makes sense, as delisted pools are no longer stored
in the `pool_registration` table.

In response to review feedback:

#2277 (comment)
@hasufell hasufell force-pushed the jospald/ADP-478/gc-delisted-stake-pools branch from 7579cfb to 226dea8 Compare November 10, 2020 09:44
@hasufell
Copy link
Contributor Author

bors try

iohk-bors bot added a commit that referenced this pull request Nov 10, 2020
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 10, 2020

try

Build failed:

  It was a bit awkward to have two completely distinct endpoints for
  this with different paths, whereas they related to exactly the same
  thing. This commit makes the API a bit more consistent by wrapping the
  `gcPoolStatus` inside a `maintenance action` object. So that, the API
  now offers two dual endpoints:

  - post maintenance action
  - get maintenance action

  There's only one maintenance action available at this stage.
@KtorZ KtorZ force-pushed the jospald/ADP-478/gc-delisted-stake-pools branch from 226dea8 to 6489807 Compare November 10, 2020 10:20
Copy link
Member

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final review done. Pushed some adjustment to the API layer to make it a bit more consistent and easier to follow from a user's perspective.

@KtorZ
Copy link
Member

KtorZ commented Nov 10, 2020

bors r+

1 similar comment
@hasufell
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 10, 2020

Already running a review

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 10, 2020

Build succeeded:

@iohk-bors iohk-bors bot merged commit 6cdad95 into master Nov 10, 2020
@iohk-bors iohk-bors bot deleted the jospald/ADP-478/gc-delisted-stake-pools branch November 10, 2020 11:55
iohk-bors bot added a commit that referenced this pull request Nov 10, 2020
2309: Fix parsing of SMASHPoolId r=hasufell a=hasufell

follow-up fix of #2249 

found by @piotr-iohk 

https://smash.shelley-qa.dev.cardano.org/api/v1/delisted vs https://smash.cardano-mainnet.iohk.io/api/v1/delisted

Co-authored-by: Julian Ospald <[email protected]>
iohk-bors bot added a commit that referenced this pull request Nov 10, 2020
2309: Fix parsing of SMASHPoolId r=hasufell a=hasufell

follow-up fix of #2249 

found by @piotr-iohk 

https://smash.shelley-qa.dev.cardano.org/api/v1/delisted vs https://smash.cardano-mainnet.iohk.io/api/v1/delisted

Co-authored-by: Julian Ospald <[email protected]>
iohk-bors bot added a commit that referenced this pull request Nov 10, 2020
2309: Fix parsing of SMASHPoolId r=hasufell a=hasufell

follow-up fix of #2249 

found by @piotr-iohk 

https://smash.shelley-qa.dev.cardano.org/api/v1/delisted vs https://smash.cardano-mainnet.iohk.io/api/v1/delisted

Co-authored-by: Julian Ospald <[email protected]>
iohk-bors bot added a commit that referenced this pull request Nov 10, 2020
2309: Fix parsing of SMASHPoolId r=hasufell a=hasufell

follow-up fix of #2249 

found by @piotr-iohk 

https://smash.shelley-qa.dev.cardano.org/api/v1/delisted vs https://smash.cardano-mainnet.iohk.io/api/v1/delisted

Co-authored-by: Julian Ospald <[email protected]>
iohk-bors bot added a commit that referenced this pull request Nov 10, 2020
2309: Fix parsing of SMASHPoolId r=hasufell a=hasufell

follow-up fix of #2249 

found by @piotr-iohk 

https://smash.shelley-qa.dev.cardano.org/api/v1/delisted vs https://smash.cardano-mainnet.iohk.io/api/v1/delisted

Co-authored-by: Julian Ospald <[email protected]>
iohk-bors bot added a commit that referenced this pull request Nov 11, 2020
2309: Fix parsing of SMASHPoolId r=hasufell a=hasufell

follow-up fix of #2249 

found by @piotr-iohk 

https://smash.shelley-qa.dev.cardano.org/api/v1/delisted vs https://smash.cardano-mainnet.iohk.io/api/v1/delisted

Co-authored-by: Julian Ospald <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants